-
Notifications
You must be signed in to change notification settings - Fork 295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test-validator: Verify program ELFs #4192
base: master
Are you sure you want to change the base?
test-validator: Verify program ELFs #4192
Conversation
let mut feature_set = FeatureSet::all_enabled(); | ||
for feature in &config.deactivate_feature_set { | ||
feature_set.deactivate(feature); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplicates a similar logic below:
agave/test-validator/src/lib.rs
Lines 859 to 870 in 3deac72
// Only activate features which are not explicitly deactivated. | |
let mut feature_set = FeatureSet::default().inactive; | |
for feature in &config.deactivate_feature_set { | |
if feature_set.remove(feature) { | |
info!("Feature for {:?} deactivated", feature) | |
} else { | |
warn!( | |
"Feature {:?} set for deactivation is not a known Feature public key", | |
feature, | |
) | |
} | |
} |
I wanted to avoid making changes to the existing code to make it easier to review. Would it be better to refactor this to avoid duplicating the logic, or should I keep it as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think it might be better to add a little refactoring like you said. It looks to me like you could slide the block you've linked up a bit, refactor it to use a FeatureSet
instance, and then plug a reference to that into the ELF's program runtime environment. Then we can use the same FeatureSet
instance to activate features in the genesis config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes sense to me! The silent failure and the misleading runtime error are a major pain for any devs using the test validator. I also can't think of a good reason to make the ELF verification opt-in, since it's supposed to be a deployed program. Thanks for putting this together!
let mut feature_set = FeatureSet::all_enabled(); | ||
for feature in &config.deactivate_feature_set { | ||
feature_set.deactivate(feature); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think it might be better to add a little refactoring like you said. It looks to me like you could slide the block you've linked up a bit, refactor it to use a FeatureSet
instance, and then plug a reference to that into the ELF's program runtime environment. Then we can use the same FeatureSet
instance to activate features in the genesis config.
Problem
solana-test-validator
doesn't check whether program ELFs are valid before appending their data to the program data accounts:agave/test-validator/src/lib.rs
Lines 798 to 808 in 3deac72
Summary of changes
Verify program ELFs before adding program data accounts to the genesis configuration.
With this change, the behavior with an invalid program ELF is the same as the
solana program deploy
command. An example output of trying to load an invalid ELF (with mutablestatic
data):Fixes #4191